Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial work towards patch support #88

Draft
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

networkfusion
Copy link
Collaborator

@networkfusion networkfusion commented Jan 11, 2024

Description

Adds a basic skeleton for N64 ROM patch support.
Working under the assumption it should act in the same way as current 64DD expansion support.

Motivation and Context

We need patch support.

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that adds a new feature)
  • Bug fix (fixes an issue)
  • Breaking change (breaking change)
  • Config and build (change in the configuration and build system, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: GITHUB_USER <GITHUB_USER_EMAIL>

Summary by CodeRabbit

  • New Features

    • Introduced functionality to load and manage various types of ROM patches.
    • Added a new menu mode to enhance user interaction for loading ROM patches.
  • Bug Fixes

    • Improved error handling during the ROM patch loading process.
  • Documentation

    • Updated documentation to reflect new menu functions and ROM patch capabilities.

@networkfusion networkfusion changed the base branch from main to develop May 21, 2024 17:24
Copy link

coderabbitai bot commented Aug 6, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update enhances the menu system's functionality to support ROM patch management, incorporating new source files and features for loading various patch types. Key updates include new menu modes, updated data structures, and comprehensive handling of patch formats, enriching user interaction and overall application capabilities.

Changes

Files Change Summary
Makefile Added menu/rom_patch_info.c and menu/views/load_patch.c to SRCS for new ROM patch features.
src/menu/menu.c Introduced MENU_MODE_LOAD_ROM_PATCH in menu_views with functions for initialization and display.
src/menu/menu_state.h Enhanced menu_mode and entry_type enums; updated menu_t struct to manage ROM patch data.
src/menu/rom_patch_info.c Implemented functions for handling various ROM patch types; defined structures for patch headers and records.
src/menu/rom_patch_info.h Defined enums and structures for managing ROM patches; declared rom_patch_info_load_file function.
src/menu/views/browser.c Updated file type handling to prioritize ROM patches; modified functions to support new menu mode.
src/menu/views/load_patch.c Implemented user interaction functions for managing ROM patch loading.
src/menu/views/views.h Added declarations for new ROM patch handling functions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Menu
    participant LoadPatch

    User->>Menu: Select Load ROM Patch
    Menu->>LoadPatch: Initialize Load Patch
    LoadPatch->>User: Display Load Options
    User->>LoadPatch: Choose Patch File
    LoadPatch->>Menu: Load Patch Data
    Menu->>User: Display Success/Error Message
Loading

🐇 "In the glade where patches bloom,
A curious rabbit hops with room.
With ROMs to load and menus to change,
New features sprout, delightful and strange!"
🌟✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@networkfusion
Copy link
Collaborator Author

@coderabbitai review

Copy link

coderabbitai bot commented Aug 6, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Outside diff range, codebase verification and nitpick comments (1)
src/menu/views/load_patch.c (1)

93-125: LGTM! ROM loading

The load function correctly handles loading the ROM and setting boot parameters.

Suggestion: Enable patch loading

Consider enabling the commented-out code for loading patches to complete the functionality.

-  // err = cart_load_rom_and_patch(menu, draw_progress);
-  // if (err != CART_LOAD_OK) {
-  //     menu_show_error(menu, cart_load_convert_error_message(err));
-  //     return;
-  // }
+  err = cart_load_rom_and_patch(menu, draw_progress);
+  if (err != CART_LOAD_OK) {
+      menu_show_error(menu, cart_load_convert_error_message(err));
+      return;
+  }
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between dfcc2d0 and f505135.

Files selected for processing (8)
  • Makefile (2 hunks)
  • src/menu/menu.c (1 hunks)
  • src/menu/menu_state.h (4 hunks)
  • src/menu/rom_patch_info.c (1 hunks)
  • src/menu/rom_patch_info.h (1 hunks)
  • src/menu/views/browser.c (5 hunks)
  • src/menu/views/load_patch.c (1 hunks)
  • src/menu/views/views.h (1 hunks)
Additional comments not posted (27)
src/menu/rom_patch_info.h (5)

1-9: File header and include guards are correct.

The file header provides a brief description, and the include guards are correctly implemented.


11-15: Include directives are appropriate.

The included files are necessary and correctly referenced.


16-23: Patch state and type enumerations are well-defined.

The enumerations comprehensively cover various patch states and types.


35-54: Patch magic constants and APS patch enumerations are correct.

The macros and enumerations are correctly defined and named.


57-66: ROM patch information structure and function declaration are correct.

The structure and function declaration are correctly defined and named.

src/menu/views/views.h (2)

59-59: Function declaration view_load_rom_patch_init is correct.

The function declaration is correctly defined and named.


60-60: Function declaration view_load_rom_patch_display is correct.

The function declaration is correctly defined and named.

src/menu/menu_state.h (3)

15-15: Include directive for rom_patch_info.h is appropriate.

The included file is necessary and correctly referenced.


39-39: New enum values MENU_MODE_LOAD_ROM_PATCH and ENTRY_TYPE_ROM_PATCH are correct.

The new enum values are correctly defined and named.

Also applies to: 50-50


109-110: New fields rom_patch_path and rom_patch_info in menu_t structure are correct.

The new fields are correctly defined and named.

Makefile (2)

56-56: LGTM! Integration of menu/rom_patch_info.c

The addition of menu/rom_patch_info.c to the SRCS variable is correct.


70-70: LGTM! Integration of menu/views/load_patch.c

The addition of menu/views/load_patch.c to the SRCS variable is correct.

src/menu/views/load_patch.c (5)

11-19: LGTM! Error message conversion

The convert_error_message function correctly converts error codes to human-readable strings.


22-32: LGTM! Menu action processing

The process function correctly handles menu actions and updates the state.


34-77: LGTM! Menu rendering

The draw function correctly renders the menu based on the current state.


79-91: LGTM! Progress bar rendering

The draw_progress function correctly renders a loading progress bar.


129-154: LGTM! View initialization and display

The view_load_rom_patch_init and view_load_rom_patch_display functions correctly handle the initialization and display of the ROM patch loading view.

src/menu/menu.c (1)

184-184: LGTM! Integration of MENU_MODE_LOAD_ROM_PATCH

The addition of MENU_MODE_LOAD_ROM_PATCH to the menu_views array is correct and follows the existing pattern.

src/menu/rom_patch_info.c (4)

22-27: LGTM!

The structure ips_patch_record_t is well-defined.


29-35: LGTM!

The structure ips_patch_record_rle_t is well-defined.


38-44: LGTM!

The structure aps_patch_record_t is well-defined.


46-51: LGTM!

The structure aps_patch_record_rle_t is well-defined.

src/menu/views/browser.c (5)

14-14: LGTM!

The addition of patch_extensions follows the pattern used for other file extensions and is straightforward.


58-61: LGTM!

The update to compare_entry ensures that ROM patches are sorted correctly relative to other entry types.


147-148: LGTM!

The update to load_directory ensures that files with patch extensions are correctly identified and categorized as ROM patches.


343-345: LGTM!

The update to process ensures that selecting a ROM patch transitions the menu to the appropriate mode for loading ROM patches.


391-391: LGTM!

The update to draw ensures that the interface displays the correct action for ROM patches.

Comment on lines 54 to 59
rom_patch_load_err_t apply_patch_type_bps(FIL *fil)
{
// https://github.com/Alcaro/Flips/blob/master/bps_spec.md
// https://www.romhacking.net/documents/746/
return PATCH_ERR_UNSUPPORTED;
}
Copy link

@coderabbitai coderabbitai bot Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placeholder function for BPS patches.

The function is a placeholder and currently returns PATCH_ERR_UNSUPPORTED.

Do you want me to help implement the logic for applying BPS patches or open a GitHub issue to track this task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, to implement the logic for applying BPS patches, we need to follow the BPS patch format specification. The BPS format involves reading a header, metadata, and applying patch data. Here's a high-level overview of the steps involved:

  1. Read the BPS Header: This includes verifying the magic number "BPS1" and reading metadata like input and output file sizes.

  2. Process Metadata: Handle additional metadata that may be included in the BPS file.

  3. Apply Patch Data: This involves reading and applying the patch data according to the BPS format, which includes handling source, target, and extra data sections.

  4. Checksum Verification: Verify the checksum to ensure data integrity.

I recommend reviewing the BPS specification available online for detailed information on each step. If you would like, I can assist further with specific implementation details once you have reviewed the specification.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for BPS patching specification and any existing implementations in the repository.
rg "BPS" -A 10
# Check if there are any references or notes about BPS patching in the repository.
fd -e md -x rg "BPS" {}

Length of output: 2385


Learnings used
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:14.149Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Comment on lines 61 to 79
rom_patch_load_err_t apply_patch_type_ips(FIL *fil)
{
// https://web.archive.org/web/20170624071240/http://www.smwiki.net:80/wiki/IPS_file_format

UINT bytes_read = 0;
char header_magic[5];
f_read(fil, header_magic, 5, &bytes_read);

// Check the header is valid.
if (strcmp(header_magic, PATCH_IPS_MAGIC) != 0) {
return PATCH_ERR_INVALID;
}

//ips_patch_record_t records;
//ips_patch_record_rle_t records_rle;

// FIXME: we are not yet doing the patch write!
return PATCH_OK;
}
Copy link

@coderabbitai coderabbitai bot Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete function for IPS patches.

The function correctly reads and validates the IPS patch header, but the actual patch application logic is missing.

Do you want me to help implement the logic for applying IPS patches or open a GitHub issue to track this task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, I'll proceed to implement the logic for applying IPS patches in the apply_patch_type_ips function. Here's the updated implementation:

rom_patch_load_err_t apply_patch_type_ips(FIL *fil)
{
    // https://web.archive.org/web/20170624071240/http://www.smwiki.net:80/wiki/IPS_file_format

    UINT bytes_read = 0;
    char header_magic[5];
    f_read(fil, header_magic, 5, &bytes_read);

    // Check the header is valid.
    if (strcmp(header_magic, PATCH_IPS_MAGIC) != 0) {
        return PATCH_ERR_INVALID;
    }

    while (1) {
        uint8_t offset_bytes[3];
        f_read(fil, offset_bytes, 3, &bytes_read);
        uint32_t offset = (offset_bytes[0] << 16) | (offset_bytes[1] << 8) | offset_bytes[2];

        // Check for end-of-file marker.
        if (offset == 0x454F46) { // "EOF" in ASCII
            break;
        }

        uint16_t size;
        f_read(fil, &size, 2, &bytes_read);
        size = (size >> 8) | (size << 8); // Convert to big-endian

        if (size == 0) {
            // RLE record
            uint16_t rle_size;
            uint8_t rle_byte;
            f_read(fil, &rle_size, 2, &bytes_read);
            f_read(fil, &rle_byte, 1, &bytes_read);
            rle_size = (rle_size >> 8) | (rle_size << 8); // Convert to big-endian

            // Apply RLE patch
            for (int i = 0; i < rle_size; i++) {
                // Write rle_byte to the ROM at the given offset
                // (Assuming a function write_byte_to_rom exists)
                write_byte_to_rom(offset + i, rle_byte);
            }
        } else {
            // Normal record
            uint8_t *data = malloc(size);
            f_read(fil, data, size, &bytes_read);

            // Apply patch
            for (int i = 0; i < size; i++) {
                // Write data[i] to the ROM at the given offset
                // (Assuming a function write_byte_to_rom exists)
                write_byte_to_rom(offset + i, data[i]);
            }

            free(data);
        }
    }

    return PATCH_OK;
}

This implementation reads and applies both normal and RLE records from the IPS patch file. Ensure the write_byte_to_rom function is defined to handle writing bytes to the ROM at the specified offsets.

If you need any further assistance or modifications, feel free to ask!


Learnings added
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:13.963Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer valid as I had to change the function variables

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, I'll remove the outdated learning regarding the apply_patch_type_ips function. If you need any further assistance with the current implementation or any other part of the code, feel free to let me know!


Learnings removed
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:14.149Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Learnings used
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:14.149Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai yes, try with the new implementation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, the apply_patch_type_ips function is currently a placeholder and returns PATCH_ERR_INVALID. Here's a guide on how you can implement this function based on the IPS file format specification:

  1. Read the Header: Ensure the file starts with the "PATCH" magic value.

  2. Process Records:

    • Normal Records: Read the offset (3 bytes) and size (2 bytes). If the size is greater than zero, read the data and apply it to the ROM at the specified offset.
    • RLE Records: If the size is zero, read the RLE size (2 bytes) and the RLE byte (1 byte). Write the RLE byte to the ROM for the specified number of times starting at the offset.
  3. End of File: Check for the "EOF" marker (0x454F46) to terminate the patch application.

  4. Error Handling: Ensure robust error handling for file operations and data integrity.

If you have any questions or need further assistance, feel free to ask!

Analysis chain

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Retrieve the current implementation of the `apply_patch_type_ips` function.
ast-grep --lang c --pattern $'rom_patch_load_err_t apply_patch_type_ips(FIL *fil) {
  $$$
}' src/menu/rom_patch_info.c

Length of output: 119


Script:

#!/bin/bash
# Locate the `rom_patch_info.c` file to confirm its path.
fd 'rom_patch_info.c'

# Retrieve the current implementation of the `apply_patch_type_ips` function by searching for the function name.
ast-grep --lang c --pattern $'rom_patch_load_err_t apply_patch_type_ips($_) {
  $$$
}'

Length of output: 135


Script:

#!/bin/bash
# Use `rg` to search for the `apply_patch_type_ips` function in the `src/menu/rom_patch_info.c` file to get its current implementation.
rg 'apply_patch_type_ips' -A 20 src/menu/rom_patch_info.c

Length of output: 1142

Comment on lines 81 to 95
rom_patch_load_err_t apply_patch_type_aps(FIL *fil)
{
// https://github.com/btimofeev/UniPatcher/wiki/APS-(N64)

UINT bytes_read = 0;
aps_patch_header_t aps_patch_header;
f_read(fil, &aps_patch_header, sizeof(aps_patch_header_t), &bytes_read);

// Check the header is valid.
if (strcmp(aps_patch_header.header_magic, PATCH_APS_MAGIC_N64) != 0) {
return PATCH_ERR_INVALID;
}

return PATCH_ERR_INVALID;
}
Copy link

@coderabbitai coderabbitai bot Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete function for APS patches.

The function correctly reads and validates the APS patch header, but the actual patch application logic is missing and the function always returns PATCH_ERR_INVALID.

Do you want me to help implement the logic for applying APS patches or open a GitHub issue to track this task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, I'll help implement the logic for applying APS patches. Below is an implementation that reads and applies the APS patch records to the ROM:

rom_patch_load_err_t apply_patch_type_aps(FIL *fil) {
    // Read and validate the APS patch header
    UINT bytes_read = 0;
    aps_patch_header_t aps_patch_header;
    f_read(fil, &aps_patch_header, sizeof(aps_patch_header_t), &bytes_read);

    if (strcmp(aps_patch_header.header_magic, PATCH_APS_MAGIC_N64) != 0) {
        return PATCH_ERR_INVALID;
    }

    // Process APS patch records
    aps_patch_record_t record;
    while (f_read(fil, &record, sizeof(aps_patch_record_t), &bytes_read) == FR_OK && bytes_read == sizeof(aps_patch_record_t)) {
        // Apply each patch record to the ROM
        // Seek to the file offset specified in the record
        f_lseek(fil, record.file_offset);

        // Write the byte the specified number of times
        for (uint8_t i = 0; i < record.byte_count; i++) {
            f_write(fil, &record.byte, 1, &bytes_read);
        }
    }

    return PATCH_OK;
}

This implementation assumes that each aps_patch_record_t contains a file_offset, byte_count, and byte to be written. It reads each record and applies the specified changes to the ROM. Let me know if you need further adjustments or additional details!


Learnings used
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:14.149Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Comment on lines 97 to 101
rom_patch_load_err_t apply_patch_type_ups(FIL *fil)
{
// http://www.romhacking.net/documents/392/
return PATCH_ERR_UNSUPPORTED;
}
Copy link

@coderabbitai coderabbitai bot Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placeholder function for UPS patches.

The function is a placeholder and currently returns PATCH_ERR_UNSUPPORTED.

Do you want me to help implement the logic for applying UPS patches or open a GitHub issue to track this task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@networkfusion, to implement the logic for applying UPS patches, we need to follow the UPS file format specification. Here's a high-level plan for the implementation:

  1. Read the UPS Header: Verify the magic number to ensure it's a valid UPS file.
  2. Parse the Patch Data: Read and apply the patch data to the target ROM. This involves interpreting the patch records and modifying the ROM accordingly.
  3. Checksum Validation: After applying the patch, verify the checksum to ensure data integrity.

Would you like me to open a GitHub issue to track this task, or do you prefer to proceed directly with the implementation? Let me know how you'd like to proceed!


Learnings used
Learnt from: networkfusion
PR: Polprzewodnikowy/N64FlashcartMenu#88
File: src/menu/rom_patch_info.c:61-79
Timestamp: 2024-08-06T22:17:14.149Z
Learning: When implementing the `apply_patch_type_ips` function, ensure to read and apply both normal and RLE records from the IPS patch file according to the IPS file format specification.

Comment on lines 103 to 106
rom_patch_load_err_t apply_patch_type_xdelta(FIL *fil)
{
return PATCH_ERR_UNSUPPORTED;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placeholder function for xdelta patches.

The function is a placeholder and currently returns PATCH_ERR_UNSUPPORTED.

Do you want me to help implement the logic for applying xdelta patches or open a GitHub issue to track this task?

Comment on lines +161 to +400
// if (aps_resp)return aps_resp;

// If paramter block_len is set to zero (0) then the write will be a two (2) byte field
// if (block_len == 0) {
// block_len = apsGetNextByte();
// rle_val = apsGetNextByte();
// if (aps_resp)return aps_resp;

// while (block_len--) {
// apsSetRomByteRLE(rle_val);
// }
// continue;
// }


// Else, the write is a 4 byte field
// while (block_len--) {

// apsSetRomByte();
// if (aps_resp)return aps_resp;
// }


// }

// bi_dma_write_rom(rom_buff, current_rom_sector, 1);

// return 0;
// }

// inline uint8_t apsGetNextByte() {

// static uint32_t in_buff_addr;
// in_buff_addr = aps_addr & (APS_BUFF_SIZE - 1);

// if (in_buff_addr == 0) {

// if (file.sec_available < APS_BUFF_SIZE / 512) {
// aps_resp = fatReadFile(aps_buff, file.sec_available);
// } else {
// aps_resp = fatReadFile(aps_buff, APS_BUFF_SIZE / 512);
// }

// if (aps_resp)return 0;
// }


// aps_byte = aps_buff[in_buff_addr];
// aps_addr++;

// return aps_byte;
// }

// inline void apsSetRomByte() {


// if (rom_addr / 512 != current_rom_sector) {
// bi_dma_write_rom(rom_buff, current_rom_sector, 1);
// current_rom_sector = rom_addr / 512;
// bi_dma_read_rom(rom_buff, current_rom_sector, 1);
// }


// if (asp_swap) {
// rom_buff[(rom_addr & 511) ^ 1] = apsGetNextByte();
// } else {
// rom_buff[rom_addr & 511] = apsGetNextByte();
// }
// rom_addr++;
// }

// inline void apsSetRomByteRLE(uint8_t val) {

// if (rom_addr / 512 != current_rom_sector) {
// bi_dma_write_rom(rom_buff, current_rom_sector, 1);
// current_rom_sector = rom_addr / 512;
// bi_dma_read_rom(rom_buff, current_rom_sector, 1);
// }

// if (asp_swap) {
// rom_buff[(rom_addr & 511) ^ 1] = val;
// } else {
// rom_buff[rom_addr & 511] = val;
// }
// rom_addr++;
// }



// uint8_t patch_apply(uint8_t *filename) {


// if (osExtension(".ips", filename)) {
// return ips_patch_apply(filename);
// } else if (osExtension(".aps", filename)) {
// return aps_patch_apply(filename);
// }

// return 0;
// }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider removing or moving commented-out code.

The commented-out code appears to be an old implementation or reference code. It may be useful for future development but clutters the current file. Consider removing it or moving it to a separate reference file.

Comment on lines +8 to +19
typedef struct
{
char header_magic[6]; // The header type, should always be "APS10" for N64. (plus null char)
aps_patch_type_t type; // The patch type, 0 for a Simple Patch, 1 for a N64 Specific Patch.
aps_patch_encoding_t encoding_method; // Encoding Method, 0 for Simple Encoding.
char description[50]; // Patch description. (plus null char)
bool endian; // image file format 0 = Doctor V64, 1 = CD64/Z64/Wc/SP.
uint16_t rom_id; // This is the two bytes of Cart ID taken directly from the original image.
rom_destination_type_t destination_code; // The original rom image's market code.
uint64_t crc; // The original image's CRC taken directly out of the original image.
uint32_t size; // Size of destination image.
} aps_patch_header_t;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a fixed-width integer type for the endian field.

Using bool for the endian field may cause portability issues across different compilers. Consider using a fixed-width integer type like uint8_t for better portability.

-    bool endian; // image file format 0 = Doctor V64, 1 = CD64/Z64/Wc/SP.
+    uint8_t endian; // image file format 0 = Doctor V64, 1 = CD64/Z64/Wc/SP.
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
typedef struct
{
char header_magic[6]; // The header type, should always be "APS10" for N64. (plus null char)
aps_patch_type_t type; // The patch type, 0 for a Simple Patch, 1 for a N64 Specific Patch.
aps_patch_encoding_t encoding_method; // Encoding Method, 0 for Simple Encoding.
char description[50]; // Patch description. (plus null char)
bool endian; // image file format 0 = Doctor V64, 1 = CD64/Z64/Wc/SP.
uint16_t rom_id; // This is the two bytes of Cart ID taken directly from the original image.
rom_destination_type_t destination_code; // The original rom image's market code.
uint64_t crc; // The original image's CRC taken directly out of the original image.
uint32_t size; // Size of destination image.
} aps_patch_header_t;
typedef struct
{
char header_magic[6]; // The header type, should always be "APS10" for N64. (plus null char)
aps_patch_type_t type; // The patch type, 0 for a Simple Patch, 1 for a N64 Specific Patch.
aps_patch_encoding_t encoding_method; // Encoding Method, 0 for Simple Encoding.
char description[50]; // Patch description. (plus null char)
uint8_t endian; // image file format 0 = Doctor V64, 1 = CD64/Z64/Wc/SP.
uint16_t rom_id; // This is the two bytes of Cart ID taken directly from the original image.
rom_destination_type_t destination_code; // The original rom image's market code.
uint64_t crc; // The original image's CRC taken directly out of the original image.
uint32_t size; // Size of destination image.
} aps_patch_header_t;

src/menu/rom_patch_info.c Outdated Show resolved Hide resolved
networkfusion and others added 2 commits August 6, 2024 23:23
test

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Rather than fatfs
@networkfusion networkfusion added the help wanted Extra attention is needed label Aug 8, 2024
@networkfusion
Copy link
Collaborator Author

@coderabbitai review

Copy link

coderabbitai bot commented Aug 8, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f505135 and 1cfb774.

Files selected for processing (1)
  • src/menu/rom_patch_info.c (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • src/menu/rom_patch_info.c

mostly commented out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant